-
Notifications
You must be signed in to change notification settings - Fork 532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate previousInterval/nextInterval APi from IntervalCollection #18060
Deprecate previousInterval/nextInterval APi from IntervalCollection #18060
Conversation
⯅ @fluid-example/bundle-size-tests: +14 Bytes
Baseline commit: 4100024 |
@@ -78,8 +78,7 @@ export class TableDocument extends DataObject<{ Events: ITableDocumentEvents }> | |||
} | |||
|
|||
public async getRange(label: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have test coverage if you break this function?
Assuming yes, let's avoid changing the public API of SparseMatrix: you already need to make a cast of something that isn't SharedString to a SharedString.
Instead, you should create the endpoint interval index inside the DataObject here and use nextInterval
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you wanted, you could change the signature of the index creation methods to only take in SharedSegmentSequence
instead of SharedString
, which would legitimize this call and still work, but I'm not sure the distinction is something we really want long-term considering we kind of want to deprecate all of the other shared sequences. So I think just doing the cast here is superior for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't have test coverage for this method breaking, you should add such a test in table-document as it's plausible we could at some point break it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found there existed the test coverage on this method (but not tested directly). The method is invoked when the tableDocument
creates a slice. Additionally, it requires the label to obtain the intervalCollection
, which is not externally accessible. I believe it should be acceptable as long as incorporating the interval index does not break the current tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if intentionally breaking this method causes existing tests to fail, I think we're fine. Thanks for checking!
Co-authored-by: Abram Sanderson <Abram.sanderson@gmail.com>
…to remove-auto-index-in-collection-3
…to remove-auto-index-in-collection-3
Co-authored-by: Abram Sanderson <Abram.sanderson@gmail.com>
AB#5288
According to https://github.com/clarenceli-msft/FluidFramework/blob/596f777e38fbf2b7474196b4c65a80b4dfc16333/.changeset/happy-dreams-sing.md
As title, the functionality of
previousInterval
andnextInterval
are shifted toendpointIndex
. The only usage of these functionalities occurs in table-document fluid example, some workarounds are included